Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update ci & deprecate bors #225

Merged
merged 5 commits into from
Jun 4, 2023
Merged

update ci & deprecate bors #225

merged 5 commits into from
Jun 4, 2023

Conversation

Emilgardis
Copy link
Member

part of rust-embedded/svd2rust#724

remove all uses of actions-rs which use deprecated gha functions and is not maintained
removes clippy_check in favour of running clippy directly

removes clippy_check in favour of running clippy directly
@Emilgardis Emilgardis requested a review from a team as a code owner June 4, 2023 15:56
@Emilgardis
Copy link
Member Author

I notice we don't have any caching here, should we maybe add that?

@burrbull
Copy link
Member

burrbull commented Jun 4, 2023

I notice we don't have any caching here, should we maybe add that?

Don't see big sense. All checks are fast, rarely changes.

Needs MSRV bump.

burrbull
burrbull previously approved these changes Jun 4, 2023
@Emilgardis
Copy link
Member Author

Emilgardis commented Jun 4, 2023

did you change the settings here also @burrbull ? The settings right now are setup for merge_group, but that has not been implemented yet

@burrbull
Copy link
Member

burrbull commented Jun 4, 2023

did you change the settings here also @burrbull ? The settings right now are setup for merge_group, but that has not been implemented yet

Add to this PR, please.

I also cannot add clippy restriction.

@Emilgardis
Copy link
Member Author

Emilgardis commented Jun 4, 2023

I'll add it

clippy is behind build now

@Emilgardis Emilgardis changed the title update ci update ci & deprecate bors Jun 4, 2023
@Emilgardis
Copy link
Member Author

image

this is not a good way to do it, since if we update msrv we'd have to bump here also, we can fix it with a name or switching to a job with needs. Since this is only a few checks I'll name the job

@burrbull
Copy link
Member

burrbull commented Jun 4, 2023

clippy is behind build now

I don't like this. check and clippy should be different jobs.

@Emilgardis
Copy link
Member Author

clippy is check though, they do exactly the same thing, just more knobs with clippy.

@burrbull
Copy link
Member

burrbull commented Jun 4, 2023

clippy is check though, they do exactly the same thing, just more knobs with clippy.

if job fails I want to see is it build fail or just clippy.
Build must be stable. Clippy often adds new check. Possibly we even should not add clippy to merge restrictions.

@Emilgardis
Copy link
Member Author

we don't use -D warnings so there shouldn't be any issues with warnings from clippy

@Emilgardis
Copy link
Member Author

Emilgardis commented Jun 4, 2023

oh, we do do #[deny(warnings)], that's bad :3

I'll separate them

@Emilgardis
Copy link
Member Author

actually, I'd rather fix the deny first and then we can determine if we should split it, I'll let you decide how it should be afterwards

Comment on lines +41 to +45
components: clippy
- run: cargo check
env:
RUSTFLAGS: -D warnings
- run: cargo clippy
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be fine imo since clippy doesn't add "error-by-default" lints just like that without reason, most new lints are added as warn or even allow

Comment on lines -22 to -24

#![deny(warnings)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced with RUSTFLAGS=-D warnings

@burrbull burrbull added this pull request to the merge queue Jun 4, 2023
Merged via the queue into master with commit 4b9937f Jun 4, 2023
@Emilgardis Emilgardis deleted the update-actions branch June 4, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants